clamp snprintf result before fwrite in mtr_flush_with_state - #1185
aysha-afrah26 wants to merge 1 commit into
Conversation
|
any update? |
There was a problem hiding this comment.
Verified on exact head 30ee5b3447b137b4b2d207b4f773f5f81bd6e24d with an independent ASan oracle compiled directly against minitrace.cpp. The same 1,200-byte event name makes exact base c88a9f429a421b312599a07fa8902524b09bf90a abort with a stack-buffer-overflow read of 1,280 bytes from the 1,024-byte linebuf at mtr_flush_with_state() / fwrite; exact head exits 0 and caps the trace output instead. The clamp also keeps a negative formatter return from becoming a huge unsigned write length. git diff --check passes and all 15 current upstream checks are green.
AI-assisted review performed with OpenAI Codex under fallenmi direction; the exact base/head oracle and live PR state were independently verified before submission.
|
this is a third party library, we do not modify it here, but upstream first |
|
Makes sense. Sent it upstream: hrydgard/minitrace#49, same clamp against their minitrace.c. Once it lands there I can rework this to just sync the vendored copy, or close it if you'd rather pick the fix up with the next minitrace update. |
Backport of hrydgard/minitrace#49 (merged upstream as 679ac44) into the vendored copy. mtr_flush_with_state passed snprintf's would-be length to fwrite, so an event name longer than the 1024 byte line buffer made fwrite read past linebuf and copy stack bytes into the trace file. MinitraceLogger reaches this with any node name from the tree XML.
30ee5b3 to
fde7e39
Compare
|
Upstream took it: hrydgard/minitrace#49 is merged as 679ac44. I've rebased this onto master and reworded the commit as a backport of that upstream commit. The hunk is byte for byte what landed there, so the vendored copy now matches upstream on this function. I kept it to the one hunk rather than resyncing the whole file since the vendored copy has other local changes (atomic event_count, the flush_buffer handling). |
This branch has not been deployed
3 participants
Backport of hrydgard/minitrace#49, merged upstream as 679ac44, into the vendored copy in 3rdparty/minitrace. The hunk here is the same one that landed upstream.
MinitraceLogger passes node.name().c_str() to minitrace as the event name, and mtr_flush_with_state formats each event into a 1024 byte stack buffer before writing it out. The length handed to fwrite is snprintf's return value, which is the size the line would have needed rather than the size it actually wrote, so any event that overflows the buffer makes fwrite read past the end of linebuf. Node names come from the name attribute in the tree XML and nothing bounds their length, and mtr_flush() runs on every status transition, so a name of about a thousand characters is enough to reach it. The bytes read past the buffer are adjacent stack memory and they land verbatim in the trace file. Under AddressSanitizer it reports a stack-buffer-overflow read of 1081 bytes out of the 1024 byte buffer at minitrace.cpp:356.
Clamping len to what snprintf actually produced closes it, and treating a negative return as nothing-to-write also covers the MSVC _snprintf case where truncation reports -1 and fwrite would be called with (size_t)-1. The vendored file already carries local changes relative to upstream (atomic event_count, the flush_buffer handling, nullptr in the header), so this carries over just the one upstream hunk rather than resyncing the whole file. The test added to gtest_loggers fails under ASan before the change and passes after.